Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: update fluent container mount path and show them using logging #2874

Merged
merged 41 commits into from
Jun 5, 2024
Merged

fix: update fluent container mount path and show them using logging #2874

merged 41 commits into from
Jun 5, 2024

Conversation

hpohekar
Copy link
Collaborator

@hpohekar hpohekar commented May 29, 2024 •
edited
Loading

closes #2867, #2865

  1. Set CONTAINER_MOUNT_PATH = None
  2. Set CONTAINER_MOUNT_PATH = EXAMPLES_PATH using autouse fixture.
  3. host_mount_path and container_mount_path are visible to user through logger.warning()

logging information of PyFluent and Fluent workdir is shown as follows.

>>> from ansys.fluent.core import launch_fluent
>>> solver = launch_fluent()
pyfluent.launcher WARNING: Starting Fluent container mounted to /mnt/c/Users/hpohekar/pyfluent, with this path available as /mnt/pyfluent for the Fluent session running inside the container.
>>>
>>> solver.exit()
>>>

@hpohekar hpohekar linked an issue May 29, 2024 that may be closed by this pull request
Make the mapped path within container easily visible to users (either through logging or through a property at session level). #2865
Closed
prmukherj
prmukherj previously approved these changes May 29, 2024
View reviewed changes
seanpearsonuk
seanpearsonuk previously approved these changes May 29, 2024
View reviewed changes
raph-luc
raph-luc previously requested changes May 29, 2024
View reviewed changes
src/ansys/fluent/core/__init__.py Outdated Show resolved Hide resolved
@hpohekar hpohekar requested a review from raph-luc May 29, 2024 13:48
@hpohekar
Copy link
Collaborator Author

hpohekar commented May 31, 2024 •
edited
Loading

We are getting following error and CI is failing. We will have to discuss proper solution for following error. We have removed this folder using sudo rm -rf <folder_name> command because it was giving permission error while removing it.

image

https://github.com/ansys/pyfluent/actions/runs/9315232834/job/25640985375?pr=2874

@hpohekar hpohekar marked this pull request as draft May 31, 2024 08:55
@hpohekar hpohekar marked this pull request as ready for review May 31, 2024 09:41
@hpohekar hpohekar marked this pull request as draft May 31, 2024 10:23
@hpohekar hpohekar marked this pull request as ready for review May 31, 2024 10:29
@raph-luc raph-luc self-requested a review May 31, 2024 12:32
@raph-luc raph-luc dismissed stale reviews from prmukherj, seanpearsonuk, and themself May 31, 2024 12:35

Dismissing stale review

raph-luc
raph-luc reviewed May 31, 2024
View reviewed changes
tests/test_launcher.py Outdated Show resolved Hide resolved
@hpohekar hpohekar enabled auto-merge (squash) June 3, 2024 16:54
raph-luc
raph-luc reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@raph-luc raph-luc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am suggesting below that the Fluent container should be mounted to pyfluent.CONTAINER_MOUNT_PATH by default, not to pyfluent.EXAMPLES_PATH

Also renaming PYFLUENT_CONTAINER_MOUNT_PATH to CONTAINER_MOUNT_PATH

src/ansys/fluent/core/__init__.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
raph-luc
raph-luc reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@raph-luc raph-luc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that my suggestion in #2874 (comment) be considered again, using an environment variable instead of a module constant. See below for more details.

Sorry for all the back and forth.

src/ansys/fluent/core/__init__.py Outdated Show resolved Hide resolved
src/ansys/fluent/core/launcher/fluent_container.py Outdated Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
@hpohekar
Copy link
Collaborator Author

hpohekar commented Jun 4, 2024

I would suggest that my suggestion in #2874 (comment) be considered again, using an environment variable instead of a module constant. See below for more details.

Sorry for all the back and forth.

No issues at all. We need best solution and we can update the code each time. Thank you.

@hpohekar hpohekar merged commit 99ef6e9 into main Jun 5, 2024
28 checks passed
@hpohekar hpohekar deleted the fix/refactor_fluent_container_mount_paths branch June 5, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raph-luc raph-luc raph-luc approved these changes

@seanpearsonuk seanpearsonuk seanpearsonuk approved these changes

@mkundu1 mkundu1 Awaiting requested review from mkundu1 mkundu1 is a code owner

@ajain-work ajain-work Awaiting requested review from ajain-work

@dnwillia-work dnwillia-work Awaiting requested review from dnwillia-work

@h-krishnan h-krishnan Awaiting requested review from h-krishnan

@prmukherj prmukherj Awaiting requested review from prmukherj prmukherj is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@hpohekar @seanpearsonuk @mkundu1 @prmukherj @raph-luc